Skip to content

fix(coreutils-port): harden uu_app builder validation#1617

Merged
chaliy merged 1 commit into
mainfrom
2026-05-08-propose-fix-for-uu_app-validator
May 8, 2026
Merged

fix(coreutils-port): harden uu_app builder validation#1617
chaliy merged 1 commit into
mainfrom
2026-05-08-propose-fix-for-uu_app-validator

Conversation

@chaliy
Copy link
Copy Markdown
Contributor

@chaliy chaliy commented May 8, 2026

Motivation

  • The previous validator accepted single-expression uu_app() bodies that could execute side effects (e.g. std::process::Command::new(...).status().map(...).unwrap()), allowing generated Rust to run during CI or at runtime.
  • Need a minimal, focused guard that ensures only safe clap builder expressions can be emitted and prevents side-effecting chains from passing validation.

Description

  • Tightened validate_uu_app_body in crates/bashkit-coreutils-port/src/args.rs to require a clap::Command::new(...)-style root expression and improved error message.
  • Extended UuAppExprValidator with visit_expr_method_call to reject clearly unsafe chain methods and added visitors to reject closures and macros inside the builder expression.
  • Added is_disallowed_chain_method helper to enumerate disallowed side-effecting methods (status, spawn, output, map, unwrap, etc.) and made path_ends_with_command_new accept only Command::new or clap::Command::new roots.
  • Added regression test rejects_non_clap_command_root_chain to cover the single-expression std::process::Command bypass scenario.

Testing

  • Ran cargo fmt --all which completed successfully.
  • Ran cargo test -p bashkit-coreutils-port and all tests passed (17 passed, 0 failed), including the new regression test that rejects the bypass.

Codex Task

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 8, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
bashkit 8c81e7f Commit Preview URL May 08 2026, 09:34 PM

@chaliy chaliy force-pushed the 2026-05-08-propose-fix-for-uu_app-validator branch from e1d9c7f to e64b91d Compare May 8, 2026 21:21
@chaliy chaliy force-pushed the 2026-05-08-propose-fix-for-uu_app-validator branch from e64b91d to 8c81e7f Compare May 8, 2026 21:33
@chaliy chaliy merged commit ffefb31 into main May 8, 2026
16 checks passed
@chaliy chaliy deleted the 2026-05-08-propose-fix-for-uu_app-validator branch May 8, 2026 21:45
chaliy added a commit that referenced this pull request May 16, 2026
Yesterday's `Coreutils Args Drift` workflow on main went red at the
`regenerating truncate` step:

    error: unsafe uu_app body: expected a single clap Command builder
           expression, found 2 statements

Upstream `truncate` (rev 94b06d97b) now routes the Command through
`uucore::clap_localization::configure_localized_command(cmd)`, so the
body has the shape

    pub fn uu_app() -> Command {
        let cmd = Command::new("truncate")…;
        uucore::clap_localization::configure_localized_command(cmd)
            .arg(…)…
    }

The rewriter elides the localization helper (it pulls in Fluent;
bashkit doesn't need it), leaving the body as a 2-stmt `let cmd = …;
cmd.arg(…).arg(…)` form. PR #1617 had tightened the validator to a
single expression, which is what this case trips.

## Fix

Extend `validate_uu_app_body` to also accept the 2-stmt shape

    [Stmt::Local(local), Stmt::Expr(tail, None)]

with strict guards so the safety story does not weaken:

- The `let` pattern must be a plain `Pat::Ident` — no `mut`, no `ref`,
  no subpattern, no type ascription, no `let … else …`.
- The initializer must pass the same per-expression validator
  (no closures, loops, match, blocks, unsafe, async; only allowlisted
  macros; only allowlisted method names) AND chain-root at
  `Command::new(...)`, identical to the single-expression path.
- The tail must pass the per-expression validator AND its receiver
  chain must bottom out at exactly the binder identifier — not at
  another Command::new, not at a function call, not at a different
  variable. This proves the tail is just continuing the same builder
  chain.

Anything richer (tuple patterns, refutable matches, type ascription,
mutability, an extra prefix statement) still fails, keeping the
TM-INF-025 boundary closed.

## Test plan

- [x] 5 new tests in `args::tests` cover both directions: positive
  (`accepts_localized_command_let_binding` mirroring truncate's exact
  shape, asserting the wrapper is elided in the emitted body and the
  tail starts at `cmd.arg(…)`); negative (`rejects_let_mut_binding…`,
  `rejects_three_statement_uu_app`, `rejects_tail_rooted_at_other_ident`,
  `rejects_let_init_not_rooted_at_command_new`).
- [x] `cargo test -p bashkit-coreutils-port` green (32 + 5 → 37 tests).
- [x] `cargo build -p bashkit` green (existing generated
  truncate_args.rs already uses this shape; this PR only re-enables
  the codegen path that produced it).
- [x] `cargo clippy -p bashkit-coreutils-port --all-targets -- -D warnings`
  clean.
- [x] `cargo fmt --check` clean.
chaliy added a commit that referenced this pull request May 16, 2026
Yesterday's `Coreutils Args Drift` workflow on main went red at the
`regenerating truncate` step:

    error: unsafe uu_app body: expected a single clap Command builder
           expression, found 2 statements

Upstream `truncate` (rev 94b06d97b) now routes the Command through
`uucore::clap_localization::configure_localized_command(cmd)`, so the
body has the shape

    pub fn uu_app() -> Command {
        let cmd = Command::new("truncate")…;
        uucore::clap_localization::configure_localized_command(cmd)
            .arg(…)…
    }

The rewriter elides the localization helper (it pulls in Fluent;
bashkit doesn't need it), leaving the body as a 2-stmt `let cmd = …;
cmd.arg(…).arg(…)` form. PR #1617 had tightened the validator to a
single expression, which is what this case trips.

Extend `validate_uu_app_body` to also accept the 2-stmt shape

    [Stmt::Local(local), Stmt::Expr(tail, None)]

with strict guards so the safety story does not weaken:

- The `let` pattern must be a plain `Pat::Ident` — no `mut`, no `ref`,
  no subpattern, no type ascription, no `let … else …`.
- The initializer must pass the same per-expression validator
  (no closures, loops, match, blocks, unsafe, async; only allowlisted
  macros; only allowlisted method names) AND chain-root at
  `Command::new(...)`, identical to the single-expression path.
- The tail must pass the per-expression validator AND its receiver
  chain must bottom out at exactly the binder identifier — not at
  another Command::new, not at a function call, not at a different
  variable. This proves the tail is just continuing the same builder
  chain.

Anything richer (tuple patterns, refutable matches, type ascription,
mutability, an extra prefix statement) still fails, keeping the
TM-INF-025 boundary closed.

- [x] 5 new tests in `args::tests` cover both directions: positive
  (`accepts_localized_command_let_binding` mirroring truncate's exact
  shape, asserting the wrapper is elided in the emitted body and the
  tail starts at `cmd.arg(…)`); negative (`rejects_let_mut_binding…`,
  `rejects_three_statement_uu_app`, `rejects_tail_rooted_at_other_ident`,
  `rejects_let_init_not_rooted_at_command_new`).
- [x] `cargo test -p bashkit-coreutils-port` green (32 + 5 → 37 tests).
- [x] `cargo build -p bashkit` green (existing generated
  truncate_args.rs already uses this shape; this PR only re-enables
  the codegen path that produced it).
- [x] `cargo clippy -p bashkit-coreutils-port --all-targets -- -D warnings`
  clean.
- [x] `cargo fmt --check` clean.
chaliy added a commit that referenced this pull request May 16, 2026
…1628)

## Summary

Yesterday's `Coreutils Args Drift` workflow on `main` went red at the
`regenerating truncate` step:

```
error: unsafe uu_app body: expected a single clap Command builder expression, found 2 statements
```

Upstream `truncate` (rev `94b06d97b`) now routes the Command through
`uucore::clap_localization::configure_localized_command(cmd)`, so the
body has the shape:

```rust
pub fn uu_app() -> Command {
    let cmd = Command::new("truncate")…;
    uucore::clap_localization::configure_localized_command(cmd)
        .arg(…)…
}
```

The rewriter elides the localization helper (it pulls in Fluent; bashkit
doesn't need it), leaving the body as a 2-stmt `let cmd = …;
cmd.arg(…).arg(…)` form. PR #1617 had tightened the validator to a
single expression, which is what this case trips. The on-disk
`truncate_args.rs` was generated before #1617 and already uses this
exact shape, so this PR only re-enables the codegen path that produced
it.

## Fix

Extend `validate_uu_app_body` in
`crates/bashkit-coreutils-port/src/args.rs` to also accept the 2-stmt
shape `[Stmt::Local(local), Stmt::Expr(tail, None)]` with strict guards
so TM-INF-025 does not weaken:

- The `let` pattern must be a plain `Pat::Ident` — no `mut`, no `ref`,
no subpattern, no type ascription, no `let … else …`.
- The initializer must pass the same per-expression validator (no
closures, loops, match, blocks, unsafe, async; only allowlisted macros;
only allowlisted method names) AND chain-root at `Command::new(...)`,
identical to the single-expression path.
- The tail must pass the per-expression validator AND its receiver chain
must bottom out at exactly the binder identifier — not at another
`Command::new`, not at a function call, not at a different variable.

Anything richer (tuple patterns, refutable matches, type ascription,
mutability, an extra prefix statement) still fails.

## Test plan

- [x] 5 new tests in `args::tests` cover both directions:
- positive: `accepts_localized_command_let_binding` mirroring truncate's
exact shape, asserting the wrapper is elided in the emitted body and the
tail continues at `cmd.arg(…)`
- negative: `rejects_let_mut_binding_in_uu_app`,
`rejects_three_statement_uu_app`, `rejects_tail_rooted_at_other_ident`,
`rejects_let_init_not_rooted_at_command_new`
- [x] `cargo test -p bashkit-coreutils-port` green (37 tests)
- [x] `cargo build -p bashkit` green
- [x] `cargo clippy -p bashkit-coreutils-port --all-targets -- -D
warnings` clean
- [x] `cargo fmt --check` clean
- [ ] Manually dispatch `Coreutils Args Drift` after merge and confirm
`regenerating truncate` is green

---
_Generated by [Claude
Code](https://claude.ai/code/session_015ZVmCsU7hZZEpRcqa5kqpD)_
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant